-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for nodes as functions #354
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's lovely! I truly appreciate it and I'll do my best to merge it. Its also big problem in react navigation which is a bit blocking for us!
protected Object evaluate() { | ||
beginContext(); | ||
Node whatNode = mNodesManager.findNodeById(mWhatNodeID, Node.class); | ||
Object retVal = whatNode.value(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we can pass context here? And then taker advantages of memoization per context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really didn't want an external evaluation context.... To try to avoid changing too much of the code - what is the evaluation context anyways? :) What I'm using is the param node that wraps another node - and for each function call it's inner node is switched to the id of the current parameter. This way things will be correct. We only need to ensure that calling the same function twice (recursive), the function should be evaluated each time.
Why?
… 28. jul. 2019 kl. 19:04 skrev Michał Osadnik ***@***.***>:
@osdnk commented on this pull request.
In android/src/main/java/com/swmansion/reanimated/UpdateContext.java <#354 (comment)>:
> @@ -7,6 +7,7 @@
public class UpdateContext {
public long updateLoopID = 0;
+ public long contextCount = 0;
public final ArrayList<Node> updatedNodes = new ArrayList<>();
Shouldn’t updatedNode be also context-related?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#354>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGVV5EVFRPH5OVVZAI5YETQBXGRBANCNFSM4IHMOWSA>.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look here please. We currently consider all Call
nodes as children of Function
node. So if one input (Param
node) of Call node is updated, all final nodes (i.e. views) which use this Function
node in tree will be marked as updated and evaluation will be performed for all of them. It's not acceptable because only one of final node should be usually marked as updated.
Take a look: Let's assume that A is updated and A is some value related to param P in some context (let's call this context X). F is a function, which is consumed by C1 and C2, but X is only important in for C1, not C2. V1 and V2 are views
A
|
P
|
...
|
F
| \
C1 C2
| |
V1 V2
In your implementation C2 will be also marked as updated.
Please try to dig into evaluation mechanism. I help with assistance!
Thanks for the thorough explanation. I think my implementation is a little bit different than what your are describing:
This is a simple node that will be considered a child of the function node.
The JS-call for creating a proc node (the Does this make the implementation clearer? |
Could you please draw or describe what will happen if we will have two views relying on one on a Function which accept one input node (one param)? |
Ohhh, so I wonder, if we can somehow manually drop this function from native side if it will not be used any longer and then attach it manually if needed? What do you think about it? I'll happily see some imperative Also, how about this API: proc((a, b) => multiply(a,b)) How to type it then in TS? |
- Changed proc API - Added useProc hook that takes care of attach/detach - Changed proc node when used directly to attach itself (should be used as global)
I agree with @terrysahaidak. Currently everywhere I use reanimated, I have to implement some kind of memoization to avoid perf issues, and even then they aren't completely gone. This feature will help a lot with that. |
I agree with everyone, but I want to exclude it from scope of this PR. Let’s discuss, but I’m afraid of blocking this change with discussions over usages of already workable and usable code :) |
@osdnk Let's merge it then and we will test it out :) |
Well, my current use case is a onboarding transition between images, changing image and text while staying on the same screen and on reaching end triggering another set of animations. It has to run on timeout and with gestures. So I do wish to do this with reanimated and rngh, which I am attempting right now. Still, on Android it is beyond bad. Since I have at least 20 nodes that need init and running straight afterwards init and all that has to happen right after splash animations are done. |
I think I might have discovered a bug in the memoization code: const interpolate = proc((a) => add(a, 10));
const setValue = proc((a) => interpolate(a));
export default class Example extends Component {
render() {
return (
<View>
<Animated.Code>
{() => block([
call([
setValue(10),
setValue(20)
],
(args) => console.log("value", args[0], args[1]))
])}
</Animated.Code>
</View>
);
}
} Expected output: 20, 30 This is probably happening since in the evaluation the interpolate proc is called twice, and since it wraps the same node id it will return the memoized value. Should this kind of |
Change callId to be concat of met contexts, not just last one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very happy about the progress on proc nodes. Left a few comments inline
android/src/main/java/com/swmansion/reanimated/nodes/CallFuncNode.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Add CI integration (#278) Motivation In React Native Gesture Handler repo we have integration with travisCI to test if our Example apps are building and some detox e2e tests on iOS. I thought that it is not much work to add CI here as we already have config and project config is similar so it would most likely work out of the box with little tweaks. Changes Added YAML config file and Travis CI integration * Clarify set node description (#285) * Add a mock implementation for test runners (#276) I was recently using reanimated in react-navigation where I needed to mock it to be able to run it in Jest. It'll be great if the mock was in the library so we don't have to duplicate it in every project. I looked through the typescript definitions and added a mock function for almost everything. I didn't use anything specific to a test runner such as Jest, so it should be possible to use the mock with any test runner. * Create ReanimatedModule.web.js (#242) Prevents crashing when referenced in libs like react-navigation. * Fix example app's metro config on windows machines (#266) glob-to-regexp does not escape backslashes as of version 0.4 which causes issues on windows machines. replacing backslashes with slashes only on windows fixes the issue. * TVos support (#291) * Add typing for concat#0 concat#1 (#293) concat#1 can be useful to convert a numerical animated value to a string. * Update README.md (#301) * Remove react/react-native peerDependencies (#298) * 💄Refine TypeScript type for boolean functions (#305) * Get rid of Bezier's arguments check (#307) * Bump version -> 1.1.0 * Add Math.log() operator (#320) I had a need for Math.log in an Animation calculation. This code all seemed to work and I think I found all the touch points, but I'm not well versed on the native side of things so please let me know if anything needs to change. * 💄 Refine TS type (#314) * Fix android CI setup (#313) * fix(🐛): Add TS support for animated transforms (#323) * Replace forEach with for loop (#329) Since `__addChild` etc. are called so many times, using a for loop makes it faster by around 10ms at least (on iOS simulator, on device it might be even more). * Bypass animation queue for onScroll events (#312) This PR is a proof of concept on how to address issues like #160 #309. As outlined in my comment, the problem with the current batched updates are that reanimated lags behind event processing by at least one event, in some cases up to 3-4. If using something like react-native-gesture-handler this is generally not noticeable as reanimated will be responsible for positioning all elements, but when deriving an animated value from a scroll event this would cause the elements positioned by reanimated to be out of sync with the scroll view. To address the lag, this PR introduces a concept of "direct events", that will bypass the queue and apply its effects immediately. Due to my unfamiliarity with the codebase I didn't really know how to do this in a nice way without also doing a quite big refactor. This is a low invasive change to illustrate how it can be addressed, I'm happy to receive feedback on how this can be done in a cleaner way. Test plan Using @lindesvard's reproduction of the issue https://github.com/lindesvard/reanimated-scroll-perf on a physical device, applying these changes should remove the lag and stuttering. Adverse effects Since the direct effects will bypass the queue, it will not retain the same order. Given the declarative nature of reanimated I think this should be OK in most cases, but still worth mentioning. A possible workaround is to flush the queue before applying direct events. The current way to determine direct events are by event name which is not particularly specific. In the odd case of naming collision, I don't see it having profound negative effects however. * (react-native-reanimated.d.ts) fix TransformsStyle (#339) This fixes typescript error `Cannot find name 'TransformStyle'.`. * target iOS 9.0 (#346) * Make `ReanimatedModule.NAME` public (#356) This allow using the constant directly with `TurboReactPackage`. * Fix: Support optional string style types (#352) * Add support for nodes inside proc (#354) Motivation When creating multiple dynamic animations with react-native-reanimated the number of nodes that needs to be transported over the bridge is a performance challenge. One of the limitations of the current primitives offered by reanimated is the lack of function nodes. Being able to define node trees that can be reused will offer better performance. A previous attempt has been done in this pull request: #42 Implementation The suggested implementation tries to limit changes to the current source code by adding new nodes without changing the existing implementation too much. Three new nodes have been added: ParamNode CallFunctionNode FunctionNode A few changes to the NodeManager to be able to force reevaluation when calling functions have been added. The implementation uses a simple proc function in Javascript to create function nodes. Following is an example of how a function is defined and used. // Defining the function const mulXY = proc((x, y) => multipy(x, y)); // Using the function const myX = new Animated.Value(12); const myY = new Animated.Value(14); const myResult = mulXY(myX, myY); Technical The solution I have used in this implementation is to implement the ParamNode as a node that holds a reference to other nodes - returning the referenced node's value when evaluated and setting the referenced node's value when changed. Calling the function node is done using the CallFuncNode that is created when calling the function (on the JS side). When the CallFuncNode is evaluated, it performs the following steps: Start a new context Copy current argument nodes to the param nodes (using a Stack in the param node) Evaluate the function node's expression Pop the current argument nodes from the param nodes End context I have also changed NodeManager's invalidate strategy so that it will force a reevaluation of nodes when we are in a function call to avoid returning the same value for multiple functions calls with different parameter references. Using a stack in the ParamNode ensures that we can create recursive functions. Memory A single proc node should be created as a global constant in javascript since they are immediately attached and will never be removed. Platforms Both Android and iOS platform-specific code has been added in addition to the required JS code. Further plans Rewriting functionality in reanimated called often should be considered. Moving complex spring functions to functions could also be considered but is not necessarily part of this PR. Testing Convert built-in derived functionality to use proc nodes and verify that all examples work nicely. I have already done this for interpolate and easings and it seems to be working fine. Verify that no memory leaks exist. I believe that the current implementation supports caching of function evaluation - would be happy if someone else could confirm this. Co-authored-by: osdnk <[email protected]> * 💄Refine typing for cond() (#359) * Fix: jest mock should use Animated.ScrollView (#344) Ran into the issue here: satya164/react-native-tab-view#845 This is because `ScrollView` doesn't have `.getNode()` but `Animated.ScrollView` does. This change fixes the error. I think this is the correct fix. cc @simonbuerger * doc: fix broken code block (#365) replaced backticks(?) to "`" * Remove deprecated componentWillMount (#342) Refactors the create animated component removing the deprecated componentWillMount lifecycle and moving the attachProps to the constructor. * Bump version -> 1.2.0 * Update README.md (#384) Fix typo * FIX javaCompileProvider (#393) FIX for #315 * Remove prevPosition in SpringState (#386) Creates a type name that is non-specific to one animation function. This is useful when an animation state is shared between animation functions (spring + decay for instance). * add event mapping type (#388) * Add createAnimatedComponent to mock (#389) * Specify component typings as React.Component classes (#373) *Rationale* Currently `Animated.View`, `Animated.Text`, `Animated.Image`, `Animated.ScrollView` are exported as `const`ants with a `React.ComponentClass` type. This means they cannot be used as generics in (for example) `React.Ref<Animated.View>`, and that the useful `getNode()` method is not typed. PR addresses this by changing the constants to `React.Component` class exports, decorated with the `getNode()` method * migrate to AndroidX (#395) Migrated source code to use AndroidX, and Example to use React Native 0.60.5. Also removed unnecessary files like flow and buck configurations, Gradle wrapper in android, etc... Removed Geolocation module from Example on iOS. I can compile for Android and iOS versions of the example app, but metro fails to resolve react-native-reanimation from '../src/Animated' via babel configuration. **HELP**: Help make metro work for example app. * Update Example app to use RN 0.61 (#401) * Optimize constant nodes creation and cleanup (#403) This PR optimizes the way we create and cleanup value nodes. When adapting constants (e.g. when doing interpolate({ inputRange: [0,1]}) we generate new nodes for start and end of the range) we've been creating new animated value nodes. This kind of nodes can never be updated on the native side and hence there is no need to send its final value back to JS when they get detached. On top of there there are several constant values that are frequently used across the library and product code with reanimated. Those can only be instantiated once and reused in all the places. That list includes 0, 1 and -1 for now. These two changes reduce the number of nodes instantiated by one third on colors/index.js example (from 88 down to 57) and reduce the number of nodes we call getValue for by 94% (from 88 down to 5). * Update iOS Example project to build with RN 0.61 (#402) * Update android Example project build.gradle to build with RN 0.61 (#407) * fix(🐛): Fix Android bug when dealing with string animated values (#311) This PR takes @msand and @osdnk from #176 into consideration. fixes #300 * Bump version -> 1.3.0 * Added web support (#390) # Why - [Preview](https://twitter.com/Baconbrix/status/1173825701192929280?s=20) - [Demo](https://reanimated.netlify.com) # How - Enabled JS runner - Added JS to broken runner nodes: `CallFunc`, `Param` - Added support for web gesture-handler - Added expo-web to Example - Added `format` node for web color transform # Test Plan - [Example](https://reanimated.netlify.com) should run - Transitions API is not supported yet * [iOS] Fix floor() implementation (#362) fixes #316 * Fix invalid url in readme.md (example in event handling) (#418) * fixed EventArgFunc typings (#422) * 💄Export AnimateProps & AnimateStyle (#429) Exporting these types can be useful for component definition. For instance: ```ts interface FlexibleCardProps extends CardProps { style?: Animated.AnimateStyle<ImageStyle>; } export const FlexibleCard = ({ card, style }: FlexibleCardProps) => ( <Animated.Image style={[styles.flexibleContainer, style]} source={card.source} /> ); ``` ``` * bump -> 1.3.1 * Fix web support (#445) * bump -> 1.3.2 * Add tvos podspec support (#440) * 💄Refine debug() typing (#434) Non-node value crash with debug() * Bump morgan from 1.9.0 to 1.9.1 in /Example (#444) Bumps [morgan](https://github.com/expressjs/morgan) from 1.9.0 to 1.9.1. - [Release notes](https://github.com/expressjs/morgan/releases) - [Changelog](https://github.com/expressjs/morgan/blob/master/HISTORY.md) - [Commits](expressjs/morgan@1.9.0...1.9.1) Signed-off-by: dependabot[bot] <[email protected]> * Bump extend from 3.0.1 to 3.0.2 in /Example (#442) Bumps [extend](https://github.com/justmoon/node-extend) from 3.0.1 to 3.0.2. - [Release notes](https://github.com/justmoon/node-extend/releases) - [Changelog](https://github.com/justmoon/node-extend/blob/master/CHANGELOG.md) - [Commits](justmoon/node-extend@v3.0.1...v3.0.2) Signed-off-by: dependabot[bot] <[email protected]> * Bump js-yaml from 3.12.0 to 3.13.1 in /Example (#441) Bumps [js-yaml](https://github.com/nodeca/js-yaml) from 3.12.0 to 3.13.1. - [Release notes](https://github.com/nodeca/js-yaml/releases) - [Changelog](https://github.com/nodeca/js-yaml/blob/master/CHANGELOG.md) - [Commits](nodeca/js-yaml@3.12.0...3.13.1) Signed-off-by: dependabot[bot] <[email protected]> * Bump mixin-deep from 1.3.1 to 1.3.2 in /Example (#443) Bumps [mixin-deep](https://github.com/jonschlinkert/mixin-deep) from 1.3.1 to 1.3.2. - [Release notes](https://github.com/jonschlinkert/mixin-deep/releases) - [Commits](jonschlinkert/mixin-deep@1.3.1...1.3.2) Signed-off-by: dependabot[bot] <[email protected]> * fix: mock.js NOOP to allow for "new" constructor call (#439) i'm getting errors in my tests from this mock as the new Value(0) call throws for arrow functions, this seems to work when i updated my mock.js in node_modules. * 🎣Allow to pass a callback as parameter of useCode() (#408) fixes #343 This can enable us to save some unnecessary node creation but more importantly, it will enable us to benefit from the Exhaustive Deps eslint rule. This rule expect a callback as first parameter: https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L64 This rule is incredibly useful to avoid bugs when using hooks that have dependencies. * Add Jest doc to the README Following #355 (comment) it's true that there is no easily accessible documentation about how to set up Jest tests with reanimated. * Fix node dirty marking when new connection between nodes is made (#450) This change fixes the problem where a parent node were marked as dirty while adding connection between nodes instead of the children node. We should be only marking children node as if we mark parent node there is a chance that this would trigger updates in leave nodes (final nodes) that should not be affected as the children added to parent node cannot impact its value. On top of that the marking change relealed a bug where we initialized loop ID inproperly. We should be initializing it with a value that'd allow the node to be evaluated at least once. It's been working before as every node would be dirty marked. With the above change the root nodes were no longer being marked but we'd still require them to be evaluated. The invalid initial setting for last loop ID was making us skip evaluation of those parent nodes. In order to test this change I created the following sample app: https://gist.github.com/7fce6b4a891f0284baa17d0d63455495 In this app there is a button that creates a new view that hooks into an existing animated value. Before this change when the view was added we'd reevaluate all the nodes that depend on that value and hence we'd get a consol.warn being shown twice (first on the initial render and then second time when we add the view). WIth this change in the `call` node should not be evaluated more than once and only on the initial render. * Bump -> 1.4.0 * Fix ESLint dev setup and errors (#421) * Fix ESLint setup and devDependencies - Fix version of types/react-native - Update husky and lint-staged * Fix lint errors * Rewrite README.md * Format and update 04.transitions.md * Format and update 05.value.md * Format and update 07.block.md * Format and update 09.code.md * Format and update 10.event.md * Format and update 11.baseNodes/set.md * Add 11.baseNodes/log.md * Format and update 11.baseNodes/floor.md * Format and update 11.baseNodes/ceil.md * Update min, max docs * Add proc node docs * Format pages * Update README.md with latest version * Remove unnecessary assets folders * Update README.md * Remove meme :c * Fix links and move 14.backward.md to 13.declarative.md * Remove 1st person narrative in about page * Remove assets/ from component-docs config We don't need static assets right now
Why fixes "test" example - all boxes animate across the screen fixes "interpolate" example - now boxes don't jitter when you drag them fixes "colors" example - now colors interpolate smoothly rather than snap to highest RGB fixes "image viewer" example - no more jittering when the user stops panning fixes snapping to edges in "chat heads" example - doesn't fix the bug where other chat heads don't follow the main one changes "different spring configs" to go from jittery circles to not moving circles (¯\_(ツ)_/¯) doesn't fix any of the interactable examples How Adding more JS support for the features added by this PR #354 we should be more careful in the future not to add native-only features.
Motivation
When creating multiple dynamic animations with react-native-reanimated the number of nodes that needs to be transported over the bridge is a performance challenge. One of the limitations of the current primitives offered by reanimated is the lack of function nodes. Being able to define node trees that can be reused will offer better performance. A previous attempt has been done in this pull request: #42
Implementation
The suggested implementation tries to limit changes to the current source code by adding new nodes without changing the existing implementation too much. Three new nodes have been added:
ParamNode
CallFunctionNode
FunctionNode
A few changes to the NodeManager to be able to force reevaluation when calling functions have been added.
The implementation uses a simple
proc
function in Javascript to create function nodes. Following is an example of how a function is defined and used.Technical
The solution I have used in this implementation is to implement the ParamNode as a node that holds a reference to other nodes - returning the referenced node's value when evaluated and setting the referenced node's value when changed.
Calling the function node is done using the CallFuncNode that is created when calling the function (on the JS side). When the CallFuncNode is evaluated, it performs the following steps:
I have also changed NodeManager's invalidate strategy so that it will force a reevaluation of nodes when we are in a function call to avoid returning the same value for multiple functions calls with different parameter references.
Using a stack in the ParamNode ensures that we can create recursive functions.
Memory
A single proc node should be created as a global constant in javascript since they are immediately attached and will never be removed.
Platforms
Both Android and iOS platform-specific code has been added in addition to the required JS code.
Further plans
Rewriting functionality in reanimated called often should be considered. Moving complex spring functions to functions could also be considered but is not necessarily part of this PR.
Testing
Convert built-in derived functionality to use proc nodes and verify that all examples work nicely. I have already done this for
interpolate
andeasings
and it seems to be working fine. Verify that no memory leaks exist.I believe that the current implementation supports caching of function evaluation - would be happy if someone else could confirm this.